MediaDevices.produceCropTarget -> CropTarget.fromElement #50

eladalon1983 · 2022-05-20T14:05:32Z

Change the point of exposure of token-minting from:
MediaDevices.produceCropTarget()
To:
CropTarget.fromElement()

Everything else is kept as-is, to be debated and improved in subsequent PRs.

Change the point of exposure of token-minting from: MediaDevices.produceCropTarget() To: CropTarget.fromElement()

eladalon1983 · 2022-05-23T18:20:26Z

@jan-ivar has requested that Chrome should hold off on shipping Region Capture so that we may keep discussing. Both @jan-ivar and @youennf have been extremely responsive in interacting with the discussion on the various issues of this API in the last few weeks. I would be gratified if you could also be similarly responsive in reviewing PRs that solidify the consensus we have worked so hard to achieve.

CC @dontcallmedom

index.html

beaufortfrancois

I believe the sample code at https://w3c.github.io/mediacapture-region/#sample-code should be using CropTarget.fromElement() instead of mediaDevices.produceCropTarget().

The README.md file would also benefit from these changes for consistency

beaufortfrancois · 2022-05-24T12:35:29Z

index.html

@@ -151,7 +151,7 @@ <h3><dfn>CropTarget</dfn> Definition</h3>
        <pre class="idl">


The text above should be edited as well. It currently says:

CropTarget is an intentionally empty, opaque identifier that exposes nothing. Its sole purpose is to be handed to {{BrowserCaptureMediaStreamTrack/cropTo}} as input.

That much seems still correct, given that the only method is static. We'd probably not have created this CropTarget as an exposure point for fromElement if we went, say, with the UUID-based solution.

The "exposes nothing" part was what triggered me as CropTarget.fromElement() is something to me.
Your call though ;)

I could remove "exposes nothing". Would that make things clear enough, or would "intentionally empty" be confusing?

Removing exposes nothing would work for me.

To increase visibility of this change to other reviewers: I've also removed "sole".

index.html

Co-authored-by: François Beaufort <[email protected]>

eladalon1983 · 2022-05-24T13:08:26Z

I believe the sample code at https://w3c.github.io/mediacapture-region/#sample-code should be using CropTarget.fromElement() instead of mediaDevices.produceCropTarget().

Thanks for catching!
Done.

The README.md file would also benefit from these changes for consistency

I'll send a separate PR once this one is merged. I think it would require a bit more prose to be updated, so it might take a few more iterations, whereas here I hope we're mostly done.

beaufortfrancois · 2022-05-25T11:02:54Z

index.html

@@ -151,7 +151,7 @@ <h3><dfn>CropTarget</dfn> Definition</h3>
        <pre class="idl">


index.html

jan-ivar · 2022-05-26T20:58:03Z

index.html

+          </dt>
+          <dd>
+            <p>
+              Calling {{CropTarget/fromElement}} with an {{Element}} of a supported type associates


"supported type" here carries no definition, and the algorithm specifies no type validation, which means it is just confusing and should be removed.

This is prose was moved unchanged. The status quo of the document has not been altered. Let's have a separate discussion for this.

index.html

jan-ivar · 2022-05-26T21:03:49Z

index.html

+              Calling {{CropTarget/fromElement}} with an {{Element}} of a supported type associates
+              that {{Element}} with a {{CropTarget}}. This {{CropTarget}} may be used as input to
+              {{BrowserCaptureMediaStreamTrack/cropTo}}. We define a
+              <dfn>valid CropTarget</dfn> as one returned by a previous call to


This doesn't belong here which I've stated in #29 (comment), but since this is in some ways a code-move, I'll accept a separate PR to remove it, in order to break down changes into smaller PRs.

This is prose was moved unchanged. The status quo of the document has not been altered. Let's have a separate discussion for this.

index.html

Co-authored-by: Jan-Ivar Bruaroey <[email protected]>

youennf · 2022-05-27T09:55:52Z

I made this API proposal based on the consensus we achieved that all elements would be supported.
Given #51 wants to revisit this, this makes me wonder whether we should pause this PR until we resolve #51.

eladalon1983 · 2022-05-27T11:24:47Z

I made this API proposal based on the consensus we achieved that all elements would be supported. Given #51 wants to revisit this, this makes me wonder whether we should pause this PR until we resolve #51.

The current PR would still be an improvement over mediaDevices.produceCropTarget() and we should therefore merge it. Also, please note this message I have posted on #51:

@youennf, you have previously agreed HTMLElement made sense, and #13, we even expanded to Element. You made this suggestion five months ago. The API as a whole has been debated for 1.5 years by now. I do not think this qualifies as "rushed". It is the nature of our jobs that decisions must be made without full certainty. Past a certain point, additional debates are less productive than making a decision, even if that decision could end up proving less than perfectly optimal. (To clarify - I think we're making a good decision in #50.)

Let's press onwards. Many productive issues await us.

…e-region into crop_to

eladalon1983 · 2022-05-31T09:00:40Z

@jan-ivar and @youennf, PTAL.

eladalon1983 added 2 commits May 20, 2022 14:45

x

cc769c8

MediaDevices.produceCropTarget -> CropTarget.fromElement

cee2d96

Change the point of exposure of token-minting from: MediaDevices.produceCropTarget() To: CropTarget.fromElement()

eladalon1983 requested review from jan-ivar and youennf and removed request for jan-ivar May 20, 2022 14:05

eladalon1983 mentioned this pull request May 20, 2022

Why expose produceCropTarget at MediaDevices level? #11

Closed

jan-ivar requested changes May 24, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

Code review pass w3c#1

4440087

eladalon1983 requested review from jan-ivar and alvestrand May 24, 2022 08:10

beaufortfrancois reviewed May 24, 2022

View reviewed changes

eladalon1983 and others added 3 commits May 24, 2022 15:04

Update index.html

cb1d78e

Co-authored-by: François Beaufort <[email protected]>

Re-run formatter

3e0b086

Update sample code

71f998e

Code review pass w3c#2

27359e5

eladalon1983 requested a review from beaufortfrancois May 25, 2022 10:37

beaufortfrancois approved these changes May 25, 2022

View reviewed changes

jan-ivar requested changes May 26, 2022

View reviewed changes

eladalon1983 and others added 2 commits May 27, 2022 10:38

Update index.html

d5628df

Co-authored-by: Jan-Ivar Bruaroey <[email protected]>

Update index.html

e2ccdd8

Co-authored-by: Jan-Ivar Bruaroey <[email protected]>

youennf mentioned this pull request May 27, 2022

What should the spec say about elements like HTMLAudioElement? #51

Closed

eladalon1983 requested a review from jan-ivar May 31, 2022 08:16

eladalon1983 added 2 commits May 31, 2022 10:58

Add note about SecureContext

ffce3e3

Merge branch 'crop_to' of https://github.com/eladalon1983/mediacaptur…

82c3ec7

…e-region into crop_to

eladalon1983 mentioned this pull request May 31, 2022

Specify a predictable error type for unimplemented Element subtypes #57

Open

jan-ivar approved these changes Jun 2, 2022

View reviewed changes

eladalon1983 merged commit 0ae2fc3 into w3c:main Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MediaDevices.produceCropTarget -> CropTarget.fromElement #50

MediaDevices.produceCropTarget -> CropTarget.fromElement #50

eladalon1983 commented May 20, 2022 •

edited by pr-preview bot

Loading

eladalon1983 commented May 23, 2022

beaufortfrancois left a comment

beaufortfrancois May 24, 2022

eladalon1983 May 24, 2022

beaufortfrancois May 24, 2022

eladalon1983 May 24, 2022 •

edited

Loading

beaufortfrancois May 24, 2022

eladalon1983 May 24, 2022

eladalon1983 May 24, 2022

beaufortfrancois May 25, 2022

eladalon1983 commented May 24, 2022

beaufortfrancois May 25, 2022

jan-ivar May 26, 2022

eladalon1983 May 27, 2022

jan-ivar May 26, 2022

eladalon1983 May 27, 2022

youennf commented May 27, 2022

eladalon1983 commented May 27, 2022 •

edited

Loading

eladalon1983 commented May 31, 2022

		@@ -151,7 +151,7 @@ <h3><dfn>CropTarget</dfn> Definition</h3>
		<pre class="idl">

MediaDevices.produceCropTarget -> CropTarget.fromElement #50

MediaDevices.produceCropTarget -> CropTarget.fromElement #50

Conversation

eladalon1983 commented May 20, 2022 • edited by pr-preview bot Loading

eladalon1983 commented May 23, 2022

beaufortfrancois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladalon1983 May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladalon1983 commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youennf commented May 27, 2022

eladalon1983 commented May 27, 2022 • edited Loading

eladalon1983 commented May 31, 2022

eladalon1983 commented May 20, 2022 •

edited by pr-preview bot

Loading

eladalon1983 May 24, 2022 •

edited

Loading

eladalon1983 commented May 27, 2022 •

edited

Loading